Skip to content
This repository has been archived by the owner on Jun 29, 2023. It is now read-only.

Fix context data with non-String values #261

Closed
wants to merge 1 commit into from
Closed

Fix context data with non-String values #261

wants to merge 1 commit into from
+117 −2

Conversation

waldeinburg
Copy link
Contributor

@waldeinburg waldeinburg commented Feb 5, 2021 •
edited
Loading

Make sure that:

  • You have read the contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

mp911de
mp911de reviewed Feb 8, 2021
View reviewed changes
src/main/java/biz/paluch/logging/gelf/log4j2/Log4j2LogEvent.java Outdated
@@ -172,11 +173,19 @@ private String getValue(MdcMessageField field) {
@Override
public String getMdcValue(String mdcName) {
ReadOnlyStringMap contextData = logEvent.getContextData();
if (null != contextData && contextData.containsKey(mdcName)) {
return contextData.getValue(mdcName);
if (null == contextData) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about converting the value ourselves to String? Creating another map imposes quite significant allocations?

@mp911de mp911de added the type: bug A general bug label Feb 8, 2021
@waldeinburg waldeinburg force-pushed the bugfix/contextmap-with-non-string-values branch from 4b2aab2 to dda9abe Compare February 8, 2021 08:55
@waldeinburg
Copy link
Contributor Author

Ouch, you're right! I actually used String.valueOf initially, but I changed it to this bad solution when tests failed and I didn't think about that null returned "null" because I was just wrapping getValue.

@waldeinburg
Copy link
Contributor Author

I just amended the commit and force-pushed. No need to have the old solution in the history.

@waldeinburg
Copy link
Contributor Author

Because I check for null now I could of course just use value.toString instead of String.valueOf. Do you prefer the other?

@waldeinburg waldeinburg force-pushed the bugfix/contextmap-with-non-string-values branch from dda9abe to 5d1e2bb Compare February 8, 2021 09:13
@waldeinburg
Copy link
Contributor Author

Never mind, I changed it. String.valueOf is just obj == null) ? "null" : obj.toString() so it's silly to use it.

mp911de pushed a commit that referenced this pull request Mar 1, 2021
@waldeinburg @mp911de
Fix context data with non-String values #261
ef37a6c
mp911de added a commit that referenced this pull request Mar 1, 2021
@mp911de
Polishing #261
6a32ce2 
Adaopt reconfig to Log4j property caching.
@mp911de mp911de added this to the 1.15.0 milestone Mar 1, 2021
@mp911de
Copy link
Owner

mp911de commented Mar 1, 2021

Thank you for your contribution. That's merged and polished now.

@mp911de mp911de closed this Mar 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mp911de mp911de mp911de left review comments

Assignees
No one assigned
Labels
type: bug A general bug
Projects
None yet
Milestone
1.15.0
Development

Successfully merging this pull request may close these issues.
2 participants
@waldeinburg @mp911de